Merged
Conversation
nathanjcochran
commented
Dec 3, 2025
| "config_dir": tmpDir, | ||
| "releases_url": "https://cli.tigerdata.com", | ||
| "version_check_interval": float64(3600000000000), // JSON unmarshals time.Duration as nanoseconds (1 hour = 3600000000000ns) | ||
| "version_check_interval": "24h0m0s", |
Member
Author
There was a problem hiding this comment.
Even though we had a test that checked for the integer representation when marshaling to JSON, I think it was the wrong behavior. Especially because it didn't match the test for the YAML representation below, which has always used the string representation. I made the JSON and YAML tests essentially 1:1 in this PR.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR improves a couple issues with our YAML/JSON marshaling logic, which I noticed when implementing #120.
In particular, the
util.SerializeToYAMLfunction previously had anomitNull boolparameter (which was kind of poorly named imo), which would cause it to serialize the results first to JSON, then back into a Go type, and then to YAML. The purpose was to ensure we correctly marshal structs from third-party libraries or generated code that are missingyaml:struct tags, but havejson:struct tags (in particular, theomitemptytag, which causes null/empty values to be omitted). However, we were setting theomitNullparameter totruein all but a couple of cases, which meant that the vast majority of ouryaml:struct tags were actually being ignored, and were therefore completely unnecessary. It also created a weird footgun, where it you forgot to setomitNulltotrue, you could end up with YAML output that differed significantly from the corresponding JSON output. I therefore decided to remove theomitNullparameter, make the function always roundtrip to/from JSON first, and get rid of all theyaml:struct tags. This should ensure that our YAML output is always 1:1 with our JSON output (even for third-party or generated structs that only havejson:tags) and remove any confusion with regards to theyaml:tags.One of the only cases where we were setting
omitNulltofalsewas when marshaling the config struct for the sake oftiger config show. As a result, theversion_check_intervalfield was being marshaled differently in the JSON vs YAML output. In the JSON output, it was being marshaled as an integer containing nanoseconds (because time.Duration is anint64under the hood), whereas in the YAML output, it was marshaled using the time.Duration.String() format (due to how gopkg.in/yaml.v3 marshalstime.Durationvalues). Changing the behavior ofSerializeToYAMLto always roundtrip to/from JSON caused it to use the integer representation everywhere, but I actually think the string representation is the one we want (that's how it's represented in the config file itself, for example, becauseviperhas special handling for durations). I therefore added a specialutil.Durationtype that allows durations to be marshaled to JSON usingtime.Duration.String(), which make it use the string representation everywhere (i.e. in both thetiger config show -o jsonandtiger config show -o yamloutput).